Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix idempotency for empty environment #77

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix idempotency for empty environment #77

wants to merge 1 commit into from

Conversation

Dan33l
Copy link

@Dan33l Dan33l commented Sep 12, 2024

An idempotent issue is present with puppetlabs-cron_core when environment => [] .
We use this into puppet-letsencrypt :
voxpupuli/puppet-letsencrypt#360
https://github.com/voxpupuli/puppet-letsencrypt/blob/master/manifests/renew.pp#L83

environment => [] is used at least in one exemple https://github.com/puppetlabs/puppetlabs-cron_core/blob/main/spec/integration/provider/cron/crontab_spec.rb#L202So it looks expected as a possible usage. I did not found a tests that check idempotency about this usage.

This PR intent a fix of this

@Dan33l Dan33l requested a review from a team as a code owner September 12, 2024 15:18
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Dan33l Dan33l mentioned this pull request Sep 13, 2024
Open
@redat00
Copy link

redat00 commented Sep 22, 2024

Just had the same issue on my personal environment, with the letsencrypt module showing corrective change every time it runs. Your PR fixes the issue.

As a temporary fix, I'll just clone this module and make the fix manually, hopefully it gets merged soon. 🙏

Thanks for your help!

@mhashizume
Copy link
Contributor

Thank you for the PR @Dan33l , we have added it to our team's backlog to review.

@joshcooper
Copy link
Contributor

joshcooper commented Sep 30, 2024 •
edited
Loading

@Dan33l could you update the tests? It looks like they're failing because they are expecting :absent, but are receiving an empty array:

  -  :environment=>:absent,
  +  :environment=>[],

@mhashizume mhashizume added the bug Something isn't working label Oct 21, 2024
@mhashizume
Copy link
Contributor

Hi @Dan33l , thank you again for your PR.

As @joshcooper mentioned, the following should get tests passing and get this unblocked:

diff --git a/spec/unit/provider/cron/parsed_spec.rb b/spec/unit/provider/cron/parsed_spec.rb
index 2b95e93..0214213 100644
--- a/spec/unit/provider/cron/parsed_spec.rb
+++ b/spec/unit/provider/cron/parsed_spec.rb
@@ -223,7 +223,7 @@ describe Puppet::Type.type(:cron).provider(:crontab) do
         expect(parameters[0][:special]).to eq(:absent)
         expect(parameters[0][:command]).to match(%r{\$HOME/bin/daily.job >> \$HOME/tmp/out 2>&1})
         expect(parameters[0][:ensure]).to eq(:present)
-        expect(parameters[0][:environment]).to eq(:absent)
+        expect(parameters[0][:environment]).to eq([])
         expect(parameters[0][:user]).to eq(:absent)

         expect(parameters[1][:name]).to match(%r{unmanaged:\$HOME/bin/monthly-\d+})
@@ -235,7 +235,7 @@ describe Puppet::Type.type(:cron).provider(:crontab) do
         expect(parameters[1][:special]).to eq(:absent)
         expect(parameters[1][:command]).to match(%r{\$HOME/bin/monthly})
         expect(parameters[1][:ensure]).to eq(:present)
-        expect(parameters[1][:environment]).to eq(:absent)
+        expect(parameters[1][:environment]).to eq([])
         expect(parameters[1][:user]).to eq(:absent)
         expect(parameters[1][:target]).to eq('foobar')
       end
@@ -259,7 +259,7 @@ describe Puppet::Type.type(:cron).provider(:crontab) do
                        special: :absent,
                        command: '/bin/true',
                        ensure: :present,
-                       environment: :absent,
+                       environment: [],
                        user: :absent,
                        target: 'foobar',
                      },

Would you be able to update the test and sign the CLA?

Thank you!

@mhashizume
Copy link
Contributor

Hi @Dan33l , would you be able to address the concerns I raised in my last comment? Thank you

@lbdemv
Copy link

lbdemv commented Nov 20, 2024

@mhashizume would you be willing to merge a separate PR (that i would create) to fix this issues?
We would like to have this fixed and since @Dan33l seems to be unresponsive to the concerns raised about the failing tests and CLA i would redo his fix and add the necessary changes.

But i don't want do impose myself, if this is not you way to handle this type of issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Dan33l @CLAassistant @redat00 @mhashizume @joshcooper @lbdemv